Setting up ESLint #110

rezakrimi · 2020-06-25T19:09:52Z

Which problem is this PR solving?

issue npm run lint doesn't work #87

Short description of the changes

Removing TSLint
Setting up ESLint similar to the core repo
~~- None of the files are modified by the linter to keep the PR small~~

linux-foundation-easycla · 2020-06-25T19:09:56Z

The committers are authorized under a signed CLA.

✅ rezakrimi (783e2d2, 43e9f0f, 43d4c55, 7a2f229)

codecov · 2020-06-25T19:13:43Z

Codecov Report

Merging #110 into master will decrease coverage by 0.45%.
The diff coverage is 82.60%.

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
- Coverage   94.52%   94.06%   -0.46%     
==========================================
  Files          62       74      +12     
  Lines        3522     3536      +14     
  Branches      372      374       +2     
==========================================
- Hits         3329     3326       -3     
- Misses        193      210      +17

Impacted Files	Coverage Δ
...ins/node/opentelemetry-plugin-pg-pool/.eslintrc.js	`0.00% <0.00%> (ø)`
...ins/node/opentelemetry-plugin-pg-pool/src/enums.ts	`100.00% <ø> (ø)`
...s/node/opentelemetry-plugin-pg-pool/src/pg-pool.ts	`89.13% <ø> (ø)`
...ins/node/opentelemetry-plugin-pg-pool/src/utils.ts	`93.33% <ø> (ø)`
...s/node/opentelemetry-plugin-pg-pool/src/version.ts	`100.00% <ø> (ø)`
plugins/node/opentelemetry-plugin-pg/.eslintrc.js	`0.00% <0.00%> (ø)`
plugins/node/opentelemetry-plugin-pg/src/enums.ts	`100.00% <ø> (ø)`
plugins/node/opentelemetry-plugin-pg/src/pg.ts	`91.37% <ø> (ø)`
plugins/node/opentelemetry-plugin-pg/src/utils.ts	`96.96% <ø> (ø)`
...lugins/node/opentelemetry-plugin-pg/src/version.ts	`100.00% <ø> (ø)`
... and 34 more

rezakrimi · 2020-06-25T20:31:59Z

I think the lint check should install ESLint modules (parser, plugins, etc.) instead of TSLint.

dyladan · 2020-06-25T21:07:21Z

I would look at the github action in the core repo. It may help you get this figured out, and it would be easier to maintain if both repos used the same mechanism.

obecny · 2020-07-01T21:50:00Z

@rezakrimi can you please make sure the build actually passes ?

rezakrimi · 2020-07-01T23:26:23Z

@rezakrimi can you please make sure the build actually passes ?

I'm planning to update the lint checks over the weekend. However, some of the files have linting issues that are not fixable by the linter, so the check will still fail. How are we going to work around that? Do you think that fixing lint issues should be a separate issue or should it be part of this PR?

dyladan · 2020-07-02T03:12:15Z

Fixing lint issues should be a part of this PR if it is not too many errors. If it is a large number of errors it may be done in a follow up, but the follow up should be done quickly.

rezakrimi · 2020-07-05T22:32:58Z

Should I make a new PR to trigger github actions?
Also, there's a long list of linting issues and I think they should be fixed in a follow up issue.

markwolff · 2020-07-08T16:32:51Z

eslint.config.js

+    "header"
+  ],
+  extends: [
+      "./node_modules/gts",


I wonder if we could extend the eslint config from the main repo so that changes are automatically picked up after a main repo release? wdyt @dyladan? (Not in scope for this PR though!)

I wonder if there is a way to depend on the file directly in github instead of a release. Very interesting idea though. A package like @opentelemetry/eslint-config would be a welcome addition I think.

Yes we could do something like npm install --save-dev https://github.com/opentelemetry-js/opentelemetry-js/tarball/master, or just the normal cloning url

@dyladan maybe we can create some general package that can be easily shared between contrib repo and here andd maybe even a 3rd party contrib. We could put there a bit more stuff, eslint, karma config, etc. basically all things that could be shared. WDYT ?

.github/workflows/lint.yml

rezakrimi · 2020-07-10T16:55:16Z

As mentioned here: "When you create a pull request from a forked repository to the base repository, GitHub sends the pull_request event to the base repository and no pull request events occur on the forked repository."
So I think because the workflow doesn't exist in the base repository, the new github workflow isn't triggered. I tries closing and reopening this PR but it didn't help.
You can check the new lint workflow running on my branch.
I'll will move on to fixing the linting issues if that looks good.

rezakrimi · 2020-07-10T20:59:33Z

Here is a list of all the linting issues that are not fixable by the linter. Please have a look and comment on how these should be resolved.

obecny

lgtm

packages/opentelemetry-test-utils/testUtils.ts

plugins/node/opentelemetry-plugin-ioredis/src/types.ts

rezakrimi · 2020-07-14T19:09:09Z

@OlivierAlbertini Since you're the author of the DNS plugin, could you please help on resolving the issue mentioned here for that plugin?

dyladan · 2020-07-15T15:12:38Z

@rezakrimi can you update the doc to make it clear which issues you have resolved and which you have not?

rezakrimi · 2020-07-15T15:17:13Z

@rezakrimi can you update the doc to make it clear which issues you have resolved and which you have not?

I did, it's on the header for each package

rezakrimi · 2020-07-15T22:42:15Z

plugins/node/opentelemetry-plugin-dns/test/integrations/dnspromise-lookup.test.ts

-            message: error!.message,
-          },
-        });
+        if (semver.lt(process.versions.node, '9.0.0')) {


I created a new dns lookup method in this file that is compatible with node 8. The only thing is that the node INVALID_ARGUMENT code seemed to be different in node 8 so I had to make separate cases for this test. If this is fine then I'll open this PR to be merged.

Any idea why the canonical code is different between them? I would rather have behavior consistent across versions if possible

based on this it seems like it's possible to have different error codes in different major releases. The error message was the same though.

rezakrimi added 4 commits June 22, 2020 17:15

fix: setting up eslint

783e2d2

resolving merge conflicts

43e9f0f

chore: setting up linter for propagators

43d4c55

chore: adding browser linting settings for jeaeger propagator

7a2f229

rezakrimi requested review from bg451, dyladan, markwolff, mayurkale22, obecny, OlivierAlbertini, rochdev and vmarchaud as code owners June 25, 2020 19:09

rezakrimi changed the title ~~Lint~~ Setting up ESLint Jun 25, 2020

rezakrimi added 4 commits July 5, 2020 14:39

chore: updating branch

0da408b

chore: updating circleci checks

40f670b

chore: moving lint check to github actions

b86a8c3

chore: removing lint from workflows

7664f9b

markwolff reviewed Jul 8, 2020

View reviewed changes

dyladan reviewed Jul 8, 2020

View reviewed changes

.github/workflows/lint.yml Outdated Show resolved Hide resolved

rezakrimi added 2 commits July 10, 2020 15:23

chore: updating packages

cff73be

style: adding new line at the end of file

bb8a820

rezakrimi requested a review from a team July 10, 2020 15:41

rezakrimi closed this Jul 10, 2020

rezakrimi reopened this Jul 10, 2020

rezakrimi closed this Jul 10, 2020

rezakrimi added 2 commits July 10, 2020 15:46

style: fixing new line at the end of file

621a962

chore: updating github actions

c981e4a

rezakrimi reopened this Jul 10, 2020

rezakrimi added 2 commits July 10, 2020 23:37

style: running npm run lint:fix on all packages

28205a0

fix: fixing some of the linting issues & versions script

1f3b8fa

obecny approved these changes Jul 13, 2020

View reviewed changes

packages/opentelemetry-test-utils/testUtils.ts Outdated Show resolved Hide resolved

rezakrimi marked this pull request as draft July 13, 2020 23:34

naseemkullah reviewed Jul 14, 2020

View reviewed changes

plugins/node/opentelemetry-plugin-ioredis/src/types.ts Outdated Show resolved Hide resolved

rezakrimi added 2 commits July 14, 2020 00:35

fix: fixing failing test for express plugin

50a5852

style: fixing more linting issues

d3ae113

naseemkullah approved these changes Jul 14, 2020

View reviewed changes

style: adding Zone to eslint globals

7792633

rezakrimi added 2 commits July 15, 2020 22:20

fix: adding dns lookup supported by node 8

f7c2ce7

style: fixing more linting issues

3a4d6ae

rezakrimi commented Jul 15, 2020

View reviewed changes

carolinee21 mentioned this pull request Jul 16, 2020

feat: adding Koa plugin carolinee21/opentelemetry-js-contrib#1

Merged

rezakrimi marked this pull request as ready for review July 16, 2020 15:37

dyladan approved these changes Jul 21, 2020

View reviewed changes

dyladan added the internal label Jul 22, 2020

dyladan merged commit cad647f into open-telemetry:master Jul 22, 2020

evantorrie mentioned this pull request Oct 9, 2022

fix(dns): remove lookupPromise polyfill for node8 dns promise tests #1223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setting up ESLint #110

Setting up ESLint #110

rezakrimi commented Jun 25, 2020 •

edited by obecny

Loading

linux-foundation-easycla bot commented Jun 25, 2020 •

edited

Loading

codecov bot commented Jun 25, 2020 •

edited

Loading

rezakrimi commented Jun 25, 2020

dyladan commented Jun 25, 2020

obecny commented Jul 1, 2020

rezakrimi commented Jul 1, 2020

dyladan commented Jul 2, 2020

rezakrimi commented Jul 5, 2020 •

edited

Loading

markwolff Jul 8, 2020 •

edited

Loading

dyladan Jul 8, 2020

markwolff Jul 8, 2020 •

edited

Loading

obecny Jul 17, 2020

rezakrimi commented Jul 10, 2020

rezakrimi commented Jul 10, 2020

obecny left a comment

rezakrimi commented Jul 14, 2020

dyladan commented Jul 15, 2020

rezakrimi commented Jul 15, 2020

rezakrimi Jul 15, 2020

dyladan Jul 20, 2020

rezakrimi Jul 20, 2020 •

edited

Loading

Setting up ESLint #110

Setting up ESLint #110

Conversation

rezakrimi commented Jun 25, 2020 • edited by obecny Loading

Which problem is this PR solving?

Short description of the changes

linux-foundation-easycla bot commented Jun 25, 2020 • edited Loading

codecov bot commented Jun 25, 2020 • edited Loading

Codecov Report

rezakrimi commented Jun 25, 2020

dyladan commented Jun 25, 2020

obecny commented Jul 1, 2020

rezakrimi commented Jul 1, 2020

dyladan commented Jul 2, 2020

rezakrimi commented Jul 5, 2020 • edited Loading

markwolff Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

dyladan Jul 8, 2020

Choose a reason for hiding this comment

markwolff Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

obecny Jul 17, 2020

Choose a reason for hiding this comment

rezakrimi commented Jul 10, 2020

rezakrimi commented Jul 10, 2020

obecny left a comment

Choose a reason for hiding this comment

rezakrimi commented Jul 14, 2020

dyladan commented Jul 15, 2020

rezakrimi commented Jul 15, 2020

rezakrimi Jul 15, 2020

Choose a reason for hiding this comment

dyladan Jul 20, 2020

Choose a reason for hiding this comment

rezakrimi Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

rezakrimi commented Jun 25, 2020 •

edited by obecny

Loading

linux-foundation-easycla bot commented Jun 25, 2020 •

edited

Loading

codecov bot commented Jun 25, 2020 •

edited

Loading

rezakrimi commented Jul 5, 2020 •

edited

Loading

markwolff Jul 8, 2020 •

edited

Loading

markwolff Jul 8, 2020 •

edited

Loading

rezakrimi Jul 20, 2020 •

edited

Loading